EMIN AKTURK | BIRMINGHAM | MODULE-DATA-GROUPS - SPRINT 1 #754
Conversation
eminakturk
added
the
Needs Review
LonMcGregor
added
Review in progress
Sprint-1/implement/dedupe.js
Outdated
| } | ||
|
|
||
| const seen = new Set(); | ||
| return elements.filter(element => { |
There was a problem hiding this comment.
While using a filter here does work, I feel like it is perhaps not the most logical way to approach this task.
Filters are typically used to transform one array into another, here you are using it to make changes to a set. Can you think of a way to rewrite this that might make it more readable / clear what the code is intended to do?
There was a problem hiding this comment.
Thank you for your review. I have added codes that could be more logical and easier to read.
LonMcGregor
added
Reviewed
eminakturk
added
the
Needs Review
There was a problem hiding this comment.
This is an improvement to the duplicate and its easier to read. But here you are now adding to a second array as well as the set. As you are using a set, can you think of a way to use just those values in the set and not need to duplicate the values in an array as well?
LonMcGregor
removed
the
Needs Review
|
That was a great point, I adjusted the code as you wish. I hope that would be enough 👍 |
eminakturk
added
the
Needs Review
|
Good work - you are done with this sprint now, well done |
LonMcGregor
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.